Skip to content

Makefile: targeted workaround for s390x builds on c10s #1674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Makefile: targeted workaround for s390x builds on c10s #1674

wants to merge 2 commits into from
+16 −1

Conversation

jeckersb
Copy link
Collaborator

@jeckersb jeckersb commented Oct 7, 2025

Signed-off-by: John Eckersberg [email protected]

@bootc-bot bootc-bot bot requested a review from henrywang October 7, 2025 13:50
gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 7, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a workaround for s390x builds on CentOS 10 by setting the MALLOC_MMAP_MAX_ environment variable for cargo build. The implementation has a critical bug in how it retrieves OS information, and can also be improved for portability and readability. My review provides a corrected and simplified version of the logic.

@jeckersb jeckersb force-pushed the RHEL-116881-workaround branch from 91f7de1 to 4d45502 Compare October 7, 2025 13:56
@jeckersb
Copy link
Collaborator Author

jeckersb commented Oct 7, 2025

Actually I don't think this is going to help things in CI because IIUC packit is using the spec under contrib/packaging and that calls cargo directly instead of building via the Makefile.

@jeckersb
Copy link
Collaborator Author

jeckersb commented Oct 7, 2025

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 7, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a targeted workaround for s390x builds on CentOS Stream 10 by setting the MALLOC_MMAP_MAX_ environment variable. The change is applied to both the Makefile for local builds and the bootc.spec file for RPM builds. The changes are clear and address the issue described. I have one suggestion for the Makefile to improve maintainability by reducing code duplication.

cgwalters
cgwalters reviewed Oct 7, 2025
View reviewed changes
contrib/packaging/bootc.spec Outdated
Comment on lines 106 to 107
%if %new_cargo_macros
%cargo_build %{?with_rhsm:-f rhsm}
%bootc_cargo_build %{?with_rhsm:-f rhsm}
Copy link
Collaborator

@cgwalters cgwalters Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems somehow simpler to do this in shell script just above like

if test $(arch) = s390x; then
  export MALLOC_MMAP_MAX_=0
fi

or so?

Us having our own cargo macros feels like overkill.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's better. I really hate bash scripting, and I really hate rpm macros, and any time I have to combine the two (and/or Makefiles...) it usually ends poorly 😆

@cgwalters
Copy link
Collaborator

That said though, since other things are broken by this today, if that's a valid workaround, why not just ask the toolchain maintainers to ship it by default?

@jeckersb
Copy link
Collaborator Author

jeckersb commented Oct 7, 2025

That said though, since other things are broken by this today, if that's a valid workaround, why not just ask the toolchain maintainers to ship it by default?

From the Jira it seems like the confidence is not particularly high on the workaround, but if we could provide early feedback on whether it helps in our case it would be quite useful to them.

@jeckersb
Copy link
Collaborator Author

jeckersb commented Oct 7, 2025

I'm going to let this round of copr builds run before I make any more changes just to see how things turn out.

@jeckersb jeckersb force-pushed the RHEL-116881-workaround branch from 3b8a3c3 to 1b6e856 Compare October 7, 2025 15:40
cgwalters
cgwalters approved these changes Oct 7, 2025
View reviewed changes
contrib/packaging/bootc.spec

%build
# https://issues.redhat.com/browse/RHEL-116881
%if 0%{?rhel} >= 10
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this happening on c9s too? I suspect it has to do with the rust version, not the host OS version.

It seems simpler to just unconditionally do it on s390x anyways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@henrywang mentioned it happening on c9s but I haven't seen it happen yet in our CI, I figured I would be more conservative at first and if we start to see it showing up on 9 we can add it.

If it's not broken on Fedora for example I'd rather not enable the workaround there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH, sorry, my bad @jeckersb. It's CS10, not CS9. CS9 is empty my brain. Sorry about that!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less broken things = better!

@jeckersb jeckersb force-pushed the RHEL-116881-workaround branch from 1b6e856 to 64651dd Compare October 7, 2025 17:21
@jeckersb
Copy link
Collaborator Author

jeckersb commented Oct 7, 2025 •
edited
Loading

Hm looks like the change worked, but it still segfaulted even with the environment variable added

https://download.copr.fedorainfracloud.org/results/packit/bootc-dev-bootc-1674/centos-stream-10-s390x/09666012-bootc/builder-live.log.gz

Relevant (trimmed) bits:

Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.75FzzG
...
++ arch
+ test s390x = s390x
+ export MALLOC_MMAP_MAX_=0
+ MALLOC_MMAP_MAX_=0
...
+ make manpages
cargo run --package xtask -- manpages
...
error: rustc interrupted by SIGSEGV, printing backtrace
...
error: Generating manpages: Syncing man pages: Extracting CLI: Running CLI JSON dump command: command exited with non-zero code `cargo run --release --features=docgen -- internals dump-cli-json`: 101
make: *** [Makefile:45: manpages] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.75FzzG (%build)

@cgwalters cgwalters marked this pull request as draft October 7, 2025 22:26
@cgwalters
Copy link
Collaborator

Marking draft since this isn't yet doing what we want and it's not clear to me that it's really a good idea to work around it only here.

@henrywang
Copy link
Collaborator

Hi @jeckersb, the latest cs10 s390x build is passed. https://dashboard.packit.dev/jobs/copr/2855430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters approved these changes

@henrywang henrywang Awaiting requested review from henrywang

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jeckersb @cgwalters @henrywang